Conversation
hysw
force-pushed
the
dpkg
branch
4 times, most recently
from
85bd214 to
ccee0ab
Compare
hysw
force-pushed
the
dpkg
branch
2 times, most recently
from
eae670e to
27bf71f
Compare
hysw
requested review from
angela28chen,
elviscapiaq,
footballhead and
wangra-google
|
This is a partial solution to automated linux packaging. |
| @@ -0,0 +1,56 @@ | |||
| #!/bin/bash | |||
There was a problem hiding this comment.
Consider marking this script as executable
| java-version: '17' | ||
| distribution: 'temurin' | ||
| - name: Check out code | ||
| uses: actions/checkout@v3 |
There was a problem hiding this comment.
Update github action pinned version.
| # See the License for the specific language governing permissions and | ||
| # limitations under the License. | ||
|
|
||
| set -ex |
There was a problem hiding this comment.
nit: set -u will halt for undefined variables. This can catch if e.g. ANDROID_NDK_HOME is not set
| set -ex | |
| set -eux |
| pushd "${DIVE_BUILD_ROOT}/package_linux/device" | ||
| cmake -DCMAKE_TOOLCHAIN_FILE=${ANDROID_NDK_HOME}/build/cmake/android.toolchain.cmake \ | ||
| -G "Ninja" \ | ||
| -DCMAKE_MAKE_PROGRAM="ninja" \ |
There was a problem hiding this comment.
nit: Given that the generator is Ninja, I don't think we need to explicitly set the make program as well
| -DCMAKE_MAKE_PROGRAM="ninja" \ |
| -DCMAKE_FIND_ROOT_PATH_MODE_LIBRARY=NEVER \ | ||
| -DDIVE_GFXR_GRADLE_CONSOLE=plain \ | ||
| ${DIVE_ROOT} | ||
| cmake --build . --config=Debug -j |
There was a problem hiding this comment.
nit: is -j necessary with ninja? I think "all the jobs!" is the ninja default
Also since we're not using a multi-config generator, do we need --config?
| cmake --build . --config=Debug -j | |
| cmake --build . |
| cmake --build . --config Release | ||
| cmake --install . --prefix ../dive_linux --config Release |
There was a problem hiding this comment.
since we're not using the multi-config generator, do we need --config?
| cmake --build . --config Release | |
| cmake --install . --prefix ../dive_linux --config Release | |
| cmake --build . | |
| cmake --install . --prefix ../dive_linux |
| if(EXISTS "${DIVE_ANDROID_PREBUILT}") | ||
| install( | ||
| DIRECTORY "${DIVE_ANDROID_PREBUILT}/" | ||
| # TODO: fix device resources location. |
There was a problem hiding this comment.
prefer TODO with bugs so that future readers have more context
| cat build/.ninja_log | ||
| - name: Produce install artifacts | ||
| run: | | ||
| cmake --install build --prefix build/install_artifact --config Release |
There was a problem hiding this comment.
nit: since we're not using the multi-config genreator, do we need --config? Also, this is a debug build
| cmake --install build --prefix build/install_artifact --config Release | |
| cmake --install build --prefix build/install_artifact |
| touch build/package_linux/dive_android_is_prebuilt | ||
| bash scripts/package_linux.sh |
There was a problem hiding this comment.
optional: consider making dive_android_is_prebuilt an option for package_linux.sh so that it's more obvious to callers about the behavior they're going to get. It also lets the caller more easily choose behavior
| run: ls -R build/package_linux/dive_android | ||
| - name: Build | ||
| run: | | ||
| mkdir -p build/package_linux/ |
There was a problem hiding this comment.
nit: since you downloaded build/package_linux, do you need to mkdir?
| mkdir -p build/package_linux/ |
|
a more general question, we are currently saving capture files, or even crash dump file (https://github.com/google/dive/pull/638/changes#diff-ecc0a1f137a1e178eb72d83ee5750d7f896705149afb8c3fab743c2a05e97952R106) at the same folder as executables, would that be a problem with your change since the user may not have write permission with the executable install folder? |
That's a related but separate question. We should follow platform idiom for those, e.g. probably |
|
I guess with Kokoro in our internal repo, we do not need this debian packaging? |
4 participants
No description provided.